Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Prevent loss of field descriptions when converting swagger specs by using refSiblings: 'preserve' #2207

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Prevent loss of field descriptions when converting swagger specs by using refSiblings: 'preserve' #2207

wants to merge 1 commit into from
+158 −1

Conversation

davetransom
Copy link

This change resolves #2067 for converting swagger 2.0 based specs to open-api 3.0.0.

What/Why/How?

Previously, when using a swagger 2.0 file with ReDoc, the descriptions of any field using a complex type (referencing another schema) would show the description of the type, instead of the description of the field itself.

Reference

As per Mermade/oas-kit#597 (comment), swagger2openapi needs refSiblings: 'preserve' to bring additional fields into the converted open-api object.

This change makes the description field available to the fieldSchema objects, but it also needs to be merged into the field model to be shown in the UI.

  1. Alters call to swagger2openapi to use refSiblings: 'preserve'
  2. Alters models\Field.ts when deciding on the description value to use, now also considers the fieldSchema.description in addition to the existing info or schema description.
  3. Adds a new test to FieldModel.test.ts and a new swagger 2.0 fixture complexFieldDescriptions.json for testing.

Additionally, there are two options for consideration when choosing the description:

  1. Use the fieldSchema.description verbatim if it's defined, or
  2. Concatenate the field description with the schema description. However, this option might be considered unintended behaviour.

I'll update (clean) the code depending on your thoughts.

Screenshots

If you would reuse a type, say "Measurement", for both weight and volume fields, the intended description is lost i.e. see the repetitive description in the first screenshot:
176603363-52afb20c-82dd-419f-a74f-30629e1e484c

where the expected result is to show the explanation for the field:
image

Checks

  • Code is linted
  • Tested
  • All new/updated code is covered with tests

Final Notes:

  • I did have some issues trying to get the vanilla main branch to pass all tests before starting to make changes, so I've only focused on the tests applicable to this PR. I'm not sure if it's my local setup though...
  • Some of the files in the base haven't had prettier applied to them yet, so my changes to the loadAndBundleSpec.ts file would have had a lot of unrelated formatting changes to existing code. I opted out for that file only, so the diff is more representative of the change.
  • I haven't created a lot of PRs before, so please let me know if I need to change/improve anything.

@davetransom
Use refSiblings: 'preserve' when converting swagger
f0da3c5 
As per Redocly#2067, `swagger2openapi` needs `refSiblings: 'preserve'` to bring additional fields into the converted open-api object.

This makes the description field available to the `fieldSchema` objects, but it also needs to be merged into the field model to be shown in the UI.
@davetransom davetransom requested a review from a team as a code owner November 16, 2022 22:28
@davetransom davetransom requested review from Oprysk and removed request for a team November 16, 2022 22:28
@AlexVarchuk
Copy link
Collaborator

Hi @davetransom. Thanks for your contribution. We appreciate that.
it looks good but relative to OpenApi 3.0 specification additional properties in the Reference Object shall be ignored. If we'll merge that it can affect the default behavior in the existing 3.0 definitions? I am not sure about that.
@RomanHotsiy Do you have any thoughts about it?

@AlexVarchuk AlexVarchuk requested a review from RomanHotsiy March 6, 2023 18:30
@davetransom
Copy link
Author

I think that's a fair enough point @AlexVarchuk.

This does only apply to the Swagger to Open API conversion (convertSwagger2OpenAPI method), so I don't think it will affect the default behaviour of the existing 3.0 definitions (non swagger related).

  • Is the main concern that other properties could creep in from the Swagger 2.0 format and bring the unintended change in Open API 3.0?
  • Or do you mean any additional properties inserted into the Open API 3.0+ model would simply be ignored after conversion from swagger 2.0?

While I'm testing in a fairly clean swagger 2.0 format (from in-production swagger) I guess it could get pretty wild. I could look at creating some more tests which try to break the conversion.

But perhaps this boils down to putting it behind an opt-in flag, perhaps in RedocNormalizedOptions, if we weren't entirely sure, and defer to users finding issues with it.

aside: In my original commit, I added a few comments to models/Field.ts where the description is merged which also might warrant some opt-in behaviour:
https://github.com/Redocly/redoc/pull/2207/files#diff-f89b60f0bf664d1d5e47242a68f88ee4fd5f81d4c847f89287341523ee5ad1cfR101-R126

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Oprysk Oprysk Awaiting requested review from Oprysk Oprysk is a code owner automatically assigned from Redocly/keyboard-warriors

@RomanHotsiy RomanHotsiy Awaiting requested review from RomanHotsiy

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

External dependency: swagger2openapi loses descriptions on complex objects
2 participants
@davetransom @AlexVarchuk